-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support for PyCharm test debugging, and add tox environment setup #5189
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unhappy with the changes to support tox. I don't want us to have to maintain another config file that we don't use in our own workflow.
pytest.ini
Outdated
@@ -19,5 +19,3 @@ python_files = test*.py | |||
python_classes = | |||
python_functions = | |||
|
|||
# always run in parallel (requires pytest-xdist, see test-requirements.txt) | |||
addopts = -nauto --cov-append --cov-report= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you deleting this? It serves a real purpose for our typical workflow (running pytest from the command line). We're trying to get rid of runtests.py, so moving it there is not a good development IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gvanrossum this is my proposal; feedback is welcome. I feel tox fills in a void that is not solved yet, so I'm not sure what type of configs are you talking about here yet; feel free to enlighten me!
This config line contains two things set, which I disabled:
- run tests in parallel. Generally, I think tests should only be run in parallel when you want to run the entire test suite. E.g. becomes cumbersome to fix/check a single test with this. Shouldn't this be the other way around? That is single threaded by default, multi-threaded on demand?
- automatically enable coverage report. Again this seems unwarranted when running a single test (e.g. developing a new feature, writing a new test, debugging a test). Also once this is set PyCharm debugger cannot connect anymore, as both use the same mechanism to intercept calls.
I don't want to break your workflow. As pointed out in my other PR, mainly I did not make this PR earlier because I did not know what that workflow is, and did not want to break it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to document that the tests run in parallel, and how to disable that (I think setting $PYTEST_ADDOPTS to -n0
does it). Though I do agree that you probably do not want to run coverage in the general case.
Since you are in the minority (none of the other devs use PyCharm to
develop mypy AFAIK) don't you think it behooves you to find ways to turn
off the flags you don't want rather than changing the defaults for the rest
of us?
Honestly I don't care about the coverage, we turned that off in Travis
anyways because it's too slow. But I do care about the parallel option.
My problem with supporting tox is that surely once there is a tox file
we'll get PRs from others to change the tox flags and that just adds to the
review load.
Then again this argument also takes time so if one of the other devs speaks
up I'll gladly back off. :-)
|
I am using PyCharm to develop mypy approximately half of time since beginning of this year. But TBH I don't like these changes. IMO our top level directory has already too many config-related things, adding |
I had the impression people use the test runner. I made the change so that
with the test runner nothing changes, moved over the config args. I did not
knew people use pytest on its own.
…On Sun, Jun 10, 2018, 18:27 Ivan Levkivskyi ***@***.***> wrote:
Then again this argument also takes time so if one of the other devs speaks
up I'll gladly back off. :-)
I am using PyCharm to develop mypy approximately half of time since
beginning of this year. *But* TBH I don't like these changes. IMO our top
level directory has already too many config-related things, adding tox.ini
will make it only worse. Also I don'y see much value in testing on many
Python versions, the situations where something doesn't work on a given
version are quite rare, and Travis will catch them (I have Travis enabled
on my fork, so I see failures before even opening a PR).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5189 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAqIPhEtrfB_GB3Y7Ax7Rj8jeABsKaU8ks5t7VbrgaJpZM4UhtBv>
.
|
Oh and both setup.cfg and pytest.ini can be merged into tox.ini. And soon moved into pyproject.toml file. To help with clutter of files at root level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few minor points of feedback, I'll have more time tomorrow to do a more thorough review.
tox.ini
Outdated
[testenv] | ||
description = run the test driver with {basepython} | ||
deps = -r ./test-requirements.txt | ||
commands = python runtests.py -x lint -x package {posargs} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package has been replaced with self-check.
tox.ini
Outdated
[testenv:type] | ||
description = type check ourselves | ||
basepython = python3.6 | ||
commands = python runtests.py testselfcheck -p '-n0' -p '-v' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self-check
would be more consistent here.
mypy/test/testcmdline.py
Outdated
@@ -58,6 +58,14 @@ def test_python_cmdline(testcase: DataDrivenTestCase) -> None: | |||
outb = process.stdout.read() | |||
# Split output into lines. | |||
out = [s.rstrip('\n\r') for s in str(outb, 'utf8').splitlines()] | |||
|
|||
is_running_in_py_charm = "PYCHARM_HOSTED" in os.environ | |||
if is_running_in_py_charm: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to create a variable here. I'd just do if "PYCHARM_HOSTED" in os.environ: ...
@ethanhs addressed all requests |
I don't run all tests either locally. However, when Travis catches something often (especially when you're new to the code base) it helps a lot to be able to reproduce locally quickly, debug it, understand why the problem arises and being able to fix/test it locally. I suppose at the end of the day it flows into the category of different workflow between us. I had a few such instances when creating #5139. That being said, I don't want to propose anything that breaks the current core developers workflow. I'm just suggesting to extend things that potentially help new people interested in contributing. |
I've restored the parallel execution of the test suite via I still propose keeping the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I am fine with this. I don't expect I'll be switching to tox myself but I don't want to stand in the way of progress for others' workflow.
Taking another look at this now. |
cool 😎 is anyone responsible with merging it? |
I think I will defer to @ilevkivskyi since he also uses PyCharm on occasion, and this could affect his workflow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment, otherwise looks good to me.
pos = next((p for p, i in enumerate(out) if i.startswith('pydev debugger: ')), None) | ||
if pos is not None: | ||
del out[pos] # the attaching debugger message itself | ||
del out[pos] # plus the extra new line added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks quite fragile (what if the debugger output format changes?). Is it possible to configure the debugger to just not print these lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please don't rebase your PR, merge when needed, otherwise it is hard to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'll need to follow up then and amend it; I haven't found a way to disable it sadly 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
Most open source projects use tox as a way of automating project setup. It means the user can just invoke tox and that will setup automatically environments in what to develop and run tests. Additionally, PyCharm is a highly popular IDE, that allows users not that friendly to the command line to quickly become productive.
This PR makes some changes to ensure that out of box clone of mypy allows running the tests from within PyCharm (and that they pass). Plus, it exposes targets that also run inside the CI locally (e.g. running tests for Python 3.4, 3.5, 3.6, 3.7).
Once you have tox installed on a system or user level (
pip install tox
):E.g. to generate the documentation locally all one needs to do is
tox -e docs
, and can then view it by opening the HTML up from a browser inside.tox/docs_out
folder. Tox is kinda like make, but cross-platform (https://tox.readthedocs.org), written in Python and built-in isolation and virtualenv creation.These are most of the changes, I've did to help me to develop the feature #5139. Ideally this would be merged before that, leaving the PR to contain the feature only.